Prototype extension profile mapping#95
Conversation
Prototype for the extension-handling rework discussed in djc#94. A `Profile<Cmd, ReqExts>` maps a (command, request-extension tuple), for a given registry, to the response type and a tuple of response-extension types, decoupling the response extension from the request extension. The existence of an impl doubles as the compile-time capability gate. This adds `RequestExts`: a trait serializing a request-extension tuple into a single `<extension>` element. Verified byte-identical to the legacy `CommandWrapper` output. `Exts<T>`: decoded `<extension>` payload as a product of options, routing children to slots by element identity via `Deserializer::parent()`. `EppClient<C, P = NoProfile>` gains `with_profile` and `transact_profiled`, sourcing both response types from the profile. Existing `transact` is untouched.
Drive the profile-driven path through the existing mock-TCP harness.
Replace the positional `exts.0 .0` tuple indexing with typed lookup (inspired by hyper).
Add `Response::extract()` with a `FromExts` trait so several extensions can be pulled in one destructure, with required-vs-optional encoded by type: This grealy improves readability at callsites.
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Here's a quick initial round of review.
The core Profile idea seems about right? Not so sure about the further layers of stuff.
|
|
||
| /// Marker for an [`EppClient`] that has no registry [`Profile`] attached. | ||
| #[derive(Debug)] | ||
| pub struct NoProfile; |
There was a problem hiding this comment.
Yeah, might be a useful change.
| /// extensions based on the profile and then send them in the login. | ||
| /// | ||
| /// This just helped me to quickly test the profile-driven transaction API. | ||
| pub fn with_profile<P2>(self) -> EppClient<C, P2> { |
There was a problem hiding this comment.
Seems like this should just be part of new().
There was a problem hiding this comment.
I agree, see my remark in the comment.
| /// .await; | ||
| /// } | ||
| /// ``` | ||
| pub async fn transact_profiled<'c, Cmd, ReqExts>( |
There was a problem hiding this comment.
Can this replace an existing method, rather than being additive? the _profiled suffix seems pretty ugly.
There was a problem hiding this comment.
Yes, all of these duplicates (transact_profiled, ProfiledCommand) should replace the current ones. In this state I wanted to just see how the API feels and if it would work.
| /// The `<resData>` payload (usually just `Cmd::Response`). | ||
| type Response: FromXmlOwned; | ||
| /// The decoded `<extension>` payload — typically an [`Exts`] tuple. | ||
| type RespExts: FromXmlOwned; |
There was a problem hiding this comment.
Let's write this out as ResponseExtensions.
| /// Implemented for tuples up to arity 3. A local trait is used (rather than | ||
| /// `ToXml` on bare tuples) because orphan rules forbid implementing the foreign | ||
| /// `ToXml` trait for foreign tuple types. | ||
| pub trait RequestExts { |
There was a problem hiding this comment.
Let's call this RequestExtensions. Maybe this should be sealed?
| serializer.end_start()?; | ||
| self.command.serialize(None, serializer)?; | ||
|
|
||
| // This bit is important for strict EPP servers. |
There was a problem hiding this comment.
Explain why it is important, not just that it is.
| } | ||
|
|
||
| impl<Cmd: ToXml, ReqExts: RequestExts> ToXml for ProfiledCommand<'_, Cmd, ReqExts> { | ||
| fn serialize<W: std::fmt::Write + ?Sized>( |
There was a problem hiding this comment.
Nit: drop the std:: prefixes everywhere.
| /// | ||
| /// Should store a tuple of optional response extensions. | ||
| #[derive(Debug, PartialEq)] | ||
| pub struct Exts<T>(pub T); |
There was a problem hiding this comment.
Could this be private? Should not have an abbreviated name.
| } | ||
| } | ||
|
|
||
| impl<T: ExtTuple> Exts<T> { |
There was a problem hiding this comment.
I am not convinced of this and would prefer to postpone it into another PR.
Alternative suggestion: implement AsRef<T> for all the Ts in the tuple, which is more type safe.
Or, just don't implement this, and downstreams can implement their own named field structs with fields for the extensions? Seems pretty okay too.
| /// ```ignore | ||
| /// let (rgp, fee): (&RgpInfData, Option<&FeeData>) = rsp.extract()?; | ||
| /// ``` | ||
| pub fn extract<'a, E: FromExts<'a>>(&'a self) -> Result<E, MissingExtension> { |
There was a problem hiding this comment.
Let's also postpone this into a separate PR.
2 participants
This is an attempt to solve #94. It supersedes #67.
This adds:
RequestExts: helper trait to avoid empty elements (there might be other ways to solve this). Partially needed due to orphan rules.FromExts: axum-inspired extractor to make call sites more readable.I added tests and code examples to helper functions that show how this would look at call sites.
I also check how this looks internally, and it might fit nicely.